Skip to content

Stop tools before rebooting.#4630

Open
SuuperW wants to merge 1 commit intomasterfrom
tool_stop
Open

Stop tools before rebooting.#4630
SuuperW wants to merge 1 commit intomasterfrom
tool_stop

Conversation

@SuuperW
Copy link
Copy Markdown
Contributor

@SuuperW SuuperW commented Feb 15, 2026

See #4629

Check if completed:

Copy link
Copy Markdown
Member

@YoshiRulz YoshiRulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #4629 (comment) I believe this is the wrong approach.

@SuuperW SuuperW dismissed YoshiRulz’s stale review April 17, 2026 18:55

Reasons given for requested change are incorrect. User has not responded to further explanation of why the current approach is best.

Copy link
Copy Markdown
Member

@YoshiRulz YoshiRulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the avoidance of doubt, here's an elaboration of the comment I previously linked to. (The rest of the discussion regards Lua and is not relevant here.)

Implementors of IToolForm.Restart are expected to use the updated values that have just been provided:

if (ServiceInjector.UpdateServices(_emulator.ServiceProvider, tool)
&& (tool is not IExternalToolForm || ApiInjector.UpdateApis(GetOrInitApiProvider, tool)))
{
if (tool.IsActive) tool.Restart();
}

If a tool needed to unhook from a core in some way, it can do that in for example the setter method of [RequiredService] IEmulator _maybeCore. A separate Stop method would be redundant.
You're trying to sculpt the tools' lifecycle, modelled on WinForms', into something that resembles a Lua script's lifecycle. That's what I mean by "the wrong approach".

@SuuperW
Copy link
Copy Markdown
Contributor Author

SuuperW commented Apr 18, 2026

If a tool needed to unhook from a core in some way, it can do that in for example the setter method of [RequiredService] IEmulator _maybeCore. A separate Stop method would be redundant.

Wouldn't the old core already be disposed by the time that setter is called? (This would introduce additional complications if we have multiple services, but that's really an issue with using reflection which is another thing I intend to eventually fix.)
ToolManager.Restart does ServiceInjector.UpdateServices(_emulator.ServiceProvider, tool) then immediately after calls the tool's Restart method. So anything that can be done from a service property's setter could also be done in Restart (assuming the tool hasn't lost its reference to the old service). If we could access the old core from Restart, then there would not be any need for a Stop method.

You're trying to sculpt the tools' lifecycle, modelled on WinForms', into something that resembles a Lua script's lifecycle.

No I'm not. The tool's lifecycle isn't relevant here. We care about the core being disposed, not the tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants